Fix escaping sequence names in health check #128
Conversation
- Restore original lowercase test_orders table - Add separate UpperCaseOrders table to test quoted identifier handling - Add unit tests for _parse_sequence_name covering various formats - Document known limitation: sequence names with dots not supported Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c837881f48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Remove nextval and cast parts | ||
| clean_value = default_value.replace("nextval('", "").replace("'::regclass)", "") | ||
| clean_value = clean_value.replace("('", "").replace("'::text)", "") |
There was a problem hiding this comment.
For the nextval(('id_seq'::text)::regclass) format the docstring and new tests mention, the current replacement chain doesn’t actually remove the outer nextval( or the trailing ::regclass) because replace("nextval('") never matches when there’s an extra (. That leaves clean_value as something like nextval(id_seq::regclass), so _parse_sequence_name returns public, nextval(id_seq::regclass) and subsequent Identifier lookups will fail. This will also make the new test_text_cast_format fail unless the parsing logic is updated to handle the extra parentheses.
Useful? React with 👍 / 👎.
Use regex to extract sequence name from nextval() expressions.
The previous string replacement approach failed for the
nextval(('id_seq'::text)::regclass) format.
Thanks to Codex for catching this issue.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add type: ignore comment since _parse_sequence_name doesn't use sql_driver. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Fixes handling of uppercase/quoted sequence names in the health check. Previously, sequence names like
"UpperCaseOrders_id_seq"would be double-quoted when passed tosql.Identifier, causing query failures.Supersedes #94 - thanks @gsoltis for identifying the issue and initial fix!
Changes
sql.Identifiertest_orders(lowercase) test case"UpperCaseOrders"table to specifically test quoted identifier handling_parse_sequence_nameKnown Limitation
Sequence names containing literal dots (e.g.,
"my.seq") are not supported and will be incorrectly parsed. This is an edge case documented in the tests.Test Plan
pytest tests/unit/database_health/to verify unit tests pass🤖 Generated with Claude Code
Fixes #109